Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add C bindings #1537

Draft
wants to merge 35 commits into
base: dev
Choose a base branch
from
Draft

Add C bindings #1537

wants to merge 35 commits into from

Conversation

eschnett
Copy link
Contributor

No description provided.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

src/binding/c/Datatype.cpp Fixed Show fixed Hide fixed
src/binding/c/Datatype.cpp Fixed Show fixed Hide fixed
}
}

void openPMD_RecordComponent_storeChunkRaw(

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 103 lines.
return cxx_component->empty();
}

void openPMD_RecordComponent_loadChunkRaw(

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 103 lines.
@eschnett eschnett mentioned this pull request Oct 16, 2023
4 tasks
@berceanu berceanu mentioned this pull request Oct 16, 2023
@ax3l ax3l added this to the 0.16.0 milestone Oct 16, 2023
@franzpoeschel
Copy link
Contributor

franzpoeschel commented Oct 17, 2023

Hey @eschnett
Given that the approach via libcxxwrap_julia basically takes a detour via a low-level (Julia) API anyway, I think that it definitely makes sense to go via C instead, giving a much more stable and ubiquitous low-level API for bindings. I'll take a closer look at this once I find the time.

(This is responding to #1025 (comment) as well)

@eschnett
Copy link
Contributor Author

A high-level design question for a C interface is how dynamically allocated structures or arrays should be handled. I am currently using this approach:

    char* get_stuff();
    size_t get_stuffSize();

where get_stuff uses malloc to allocate memory, and get_stuffSize return the size of the allocated memory (if necessary, not necessary e.g. for strings which are NUL-terminated.) The caller needs to free the returned memory.

Another approach that could be used would be

    size_t get_stuff(char* ptr, size_t provided_size);

where the caller needs to allocate the memory. get_stuff returns the actually needed size, and writes things to ptr only if ptr is non-NULL, and if the provided_size is large enough. A variant of that is

    int get_suff(char *ptr, size_t *size);

which writes the necessary size into *size (if non-null), and writes the data into *ptr (if non-null), returning either nothing or an error code. In both cases the caller is responsible for both allocating and freeing the memory.

This choice is on one hand inconsequential – it just specifies how memory is allocated or freed – but is on the other hand pervasive throughout the API.

src/binding/c/backend/Attributable.c Fixed Show fixed Hide fixed
src/binding/c/backend/Attributable.c Fixed Show fixed Hide fixed
src/binding/c/backend/Attributable.c Fixed Show fixed Hide fixed
@franzpoeschel
Copy link
Contributor

A high-level design question for a C interface is how dynamically allocated structures or arrays should be handled. I am currently using this approach:

    char* get_stuff();
    size_t get_stuffSize();

where get_stuff uses malloc to allocate memory, and get_stuffSize return the size of the allocated memory (if necessary, not necessary e.g. for strings which are NUL-terminated.) The caller needs to free the returned memory.

Another approach that could be used would be

    size_t get_stuff(char* ptr, size_t provided_size);

where the caller needs to allocate the memory. get_stuff returns the actually needed size, and writes things to ptr only if ptr is non-NULL, and if the provided_size is large enough. A variant of that is

    int get_suff(char *ptr, size_t *size);

which writes the necessary size into *size (if non-null), and writes the data into *ptr (if non-null), returning either nothing or an error code. In both cases the caller is responsible for both allocating and freeing the memory.

This choice is on one hand inconsequential – it just specifies how memory is allocated or freed – but is on the other hand pervasive throughout the API.

I think that your current approach is fine. openPMD-api is not so low-level that memory management needs to be done by the user IMO. I'd suggest that we add either something like void openPMD_free(void *) or even structure-specific openPMD_<structname>_free(...) functions, so we can avoid future breaking changes.

Is this PR ready for review? Give me a note if you need help with anything (like CMake integration of the tests).


openPMD_Series *openPMD_Series_new();

#if openPMD_HAVE_MPI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the C++ API we're planning to split headers into parallel and non-parallel headers which would simplify shipping openPMD-api since you don't need separate MPI and non-MPI builds.
It would probably be good to start out with such a separation in the C bindings from the beginning already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll think about it. My main use case is calling openPMD_api from Julia, and there we always have MPI available.

@eschnett
Copy link
Contributor Author

@franzpoeschel The code is not yet ready for review, some functions are still missing.

@eschnett eschnett marked this pull request as draft December 22, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants